Complete Refactor to TypeScript, Improved Test Coverage, and Bug Fixes #323
Conversation
…ith the separate builds
… of coverage on CronParser, not sure why yet. Added a lot of FIXME's for things that don't make sense or aren't needed. Phase 2 basically done. Phase 3 will be a code review.
Co-authored-by: Harri Siirak <[email protected]>
|
@harrisiirak At my day job we have decided to pull the plug on using ESM. Not really an issue in this project but we have found it to be excruciatingly painful when working with a complex networking app. The biggest issue is testing but there are other issues like synthetic default imports, importing JSON, plugin scripts, file name extensions, and having to use run time flags, among others. If you want I could switch over to CJS in an hour or so for the work I have done. |
Going solely with CJS is probably less of a headache than including ESM in a hybrid build. If it doesn't require much effort, let's stick with CJS for now, and I'll consider using ESM when it becomes more mature. |
Not the most interesting example for #269 but I have been up since 3:30 am. More proof of concept as I've not used this Doc generator before. |
|
@MichaelLeeHobbs I'll try to give it a last view later today or tomorrow. It has been busy the last few days. |
There was a problem hiding this comment.
Hey @MichaelLeeHobbs!
All the recent changes you made are looking good! Added couple new comments/requests - don't have anything more to add at the moment. There are still some open threads from previous PR reviews that requires your attention. Let me know if I can be of any help or assistance to you, so that we can wrap it up soon.
I've tested and benchmarked the changes locally. Package usage is still fairly simple (we probably need to update README.md to reflect interface changes and how parseExpression can be used through CronParser).
While benchmarking the code, I discovered that there there could be some possible regressions:
start pattern: * * * * * *
old end: 2023-06-05T16:17:47.000Z
old: 1.106s
new end: Mon Jun 05 2023 19:17:47 GMT+0300 (Eastern European Summer Time)
new: 1.572s
end pattern: * * * * * *I haven't really investigated/profiled the code, but I can possibly look into the code and see if I've missed something that can possibly cause that.
Small script that I wrote for benchmarking old and new (this branch) codebase:
import * as parser from './old';
import { CronParser } from './new';
const oldParseExpression = parser.parseExpression;
const newParseExpression = CronParser.parseExpression;
function parseAndBenchMarkExpression(expression: string, iterations = 100000) {
const currentDate = new Date();
console.log(`start pattern: ${expression}`);
console.time('old');
{
const result = oldParseExpression(expression, { currentDate });
for (let i = 0, c = iterations; i < c; i++) {
result.next();
}
console.log('old end:', result.next().toDate());
}
console.timeEnd('old');
console.time('new');
{
const result = newParseExpression(expression, { currentDate });
for (let i = 0, c = iterations; i < c; i++) {
result.next();
}
console.log('new end:', result.next().toString());
}
console.timeEnd('new');
console.log(`end pattern: ${expression}`);
}
parseAndBenchMarkExpression('* * * * * *');| "generate-badges": "jest-coverage-badges", | ||
| "test:types": "npm run build && tsd", | ||
| "test": "cross-env TZ=UTC npm run lint && npm run test:types && npm run test:coverage && npm run generate-badges", | ||
| "docs": "rimraf docs && typedoc --out docs --readme none --name 'CronParser' src" |
There was a problem hiding this comment.
I was thinking about the documentation a bit. Let's remove docs/ folder from the repository at the moment. I think it potentially could create a bit too much noise in the future.
If it's required, it can be fairly easily generated locally or accessed publicly. We can automate generated documentation publishing (discussion here and a good example here) by using GH Pages action and add it to the pipeline. If you don't have time for it, I can add documentation generation automation myself.
| // var a = { | ||
| // 'second': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 0 | ||
| // ] | ||
| // }, | ||
| // 'minute': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 0, | ||
| // 30 | ||
| // ] | ||
| // }, | ||
| // 'hour': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 9 | ||
| // ] | ||
| // }, | ||
| // 'dayOfMonth': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 15 | ||
| // ] | ||
| // }, | ||
| // 'month': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 1 | ||
| // ] | ||
| // }, | ||
| // 'dayOfWeek': { | ||
| // 'wildcard': false, | ||
| // 'values': [ | ||
| // 1, | ||
| // 2, | ||
| // 3, | ||
| // 4, | ||
| // 5 | ||
| // ] | ||
| // } | ||
| // }; |
| * @memberof CronExpression | ||
| * @public | ||
| */ | ||
| next(): CronDate | { value: CronDate; done: boolean } { |
There was a problem hiding this comment.
While it's actually as same as previously, I started thinking, maybe it makes sense to separate iterator and "date" access behaviour. Having proper-proper ES6 iterator interface fully implemented is a big bonus.
With proper typings, the annoying part is that if we want to use the result of this method, we always need to assert it as CronDate. This is something that should be also reflected in the documentation.
What I'm thinking:
nextDateandprevDatefor "default" behaviournextandprevas part of iterator implementation;iteratoroption can be deprecated and removed.
Such an API change would be nice to include in a major version release since adopting this version would require a couple of changes in one way or another.
Co-authored-by: Harri Siirak <[email protected]>
Co-authored-by: Harri Siirak <[email protected]>
I'll look later but I think you have to run 1m iterations before node fully optimizes the code. I read an article on this years ago but can't find it now. In general, you ignore the first 1m calls then start your timer. This will be the best apples-to-apples comparison I believe. No matter what, we will need flame graphs. |
|
Hi @MichaelLeeHobbs! Is there any way I can help you with this PR? Since a lot of work has already been done, I still hope to see this being merged. If you really don't have any time work on this, I could in theory fork your branch and address all the open issues/questions myself and prepare final PR. Best regards |
|
@harrisiirak feel free to fork. We are in crunch time for the next couple of months. I'm working some on the weekends and quite honestly after I'm done coding for work, I don't have it in me to do even more coding. I've invited you as a collaborator on the fork. Also for your awareness, we are using the fork in production for a high-performance DICOM router. The |
| assert(atoms.length <= 6, 'Invalid cron expression, too many fields'); | ||
| const defaults = ['*', '*', '*', '*', '*', '0']; | ||
| if (atoms.length < defaults.length) { | ||
| atoms.unshift(...defaults.slice(atoms.length)); |
There was a problem hiding this comment.
| atoms.unshift(...defaults.slice(atoms.length)); | |
| atoms.unshift(...defaults.slice(atoms.length).reverse()); |
See example:
const defaults = ['*', '*', '*', '*', '*', '0'];
const atoms = ['3', '15-25', 'jan,feb', '*'];
atoms.unshift(...defaults.slice(atoms.length));
console.log(atoms);
// prints ["*", "0", "3", "15-25", "jan,feb", "*"]
// This will run every second of 0th minute, not expected
const atoms2 = ['3', '15-25', 'jan,feb', '*'];
atoms2.unshift(...defaults.slice(atoms2.length).reverse());
console.log(atoms2);
// prints ["0", "*", "3", "15-25", "jan,feb", "*"]
// This will run every minute, as expected
|
@harrisiirak @MichaelLeeHobbs Hiya, since it has been some time - what is the current status of this? I see this PR is not published on npm anywhere - is it maybe possible to just merge this in and publish a beta package only or something? |
|
Hi @sommmen! As you see from @MichaelLeeHobbs last comment, he don't have time work on it anymore. There are still some (minor) open stuff that needs be addressed before this PR can be merged. I volunteered myself (and I already forked this codebase) to solve these open issues, but I haven't really had any free time to work on it either. I'll try to find some time next week and finish it up - there aren't that much left to do, as @MichaelLeeHobbs has done a great job with this TS rewrite.
If it's ready for merge (I'll probably open up a new PR and close this one after that), I'm planning to release it as a new major version (v5). |
Sure thing - i'm not a JS myself so I can't help out but i understand it takes time - i was just curious since it was some time ago. Anyways i'm saying, from a consumers perspective, i don't mind a version that is nog 100% (feature) complete for a big rewrite like this so if its a safe build don't be shy with a new release :). Thanks for your continued support to foss software. |
|
Yeah, sorry. Way underwater. A year-long project is in chaos as one side cannot deliver the data they said they could so multiple teams are all scrambling to work around this limitation. The problem is this data element is critical as it's the catalyst that kicks off the whole process. We will be swinging back around to the project that uses cron-parser but that will likely be a few months at best. |
|
@MichaelLeeHobbs no worries, it's completely understandable. I'll let you know if I'm wrapping up with final touches and create the final draft for this TS rewrite. I was also able to find out what was degrading the performance. It appears that |
I'd say drop it and let TypeScript warn people when they are making mistakes. |
This pull request introduces a complete refactor of the codebase into TypeScript, maintaining compatibility with the documented API. Users should not experience any breaking changes, however, those relying on undocumented features may need to update their code. The update includes builds for both CommonJS (CJS) and EcmaScript Modules (ESM), as well as improved linting, enhanced test coverage to nearly 100%, and automatic documentation generation from code.
Additional changes include the implementation of a pre-commit hook to run linting and tests, the addition of numerous tests and bug fixes, and the inclusion of code coverage badges. The .npmignore file has been updated to ignore build artifacts for a lightweight library, and tsconfig files for both CJS and ESM builds have been added. The primary test has been converted to a TypeScript Jest test, with TSD tests updated for revised types. The package.json file has been updated to incorporate new scripts and dependencies. A debug library has also been added for debugging via DEBUG=cron-parser.
This pull request addresses and potentially fixes several issues (#112, #153, #156, #190, #222, #236, #269, #299, #309, #322). Issue #244 is fixed in strict mode only, and issues #273 and #309 may be resolved as well. Issue #249 is partially addressed through the implementation of strict mode, although it may not be entirely complete.
The commit history provides a detailed overview of the work in progress, including refactoring, cleanup, and updates to various components of the codebase.
This pull request still has several items that need to be addressed before it can be considered complete:
See notes.md for more details.